fix(copy): add support for copying Blob objects
#14064
Conversation
Although `copy()` does not need to (and never will) support all kinds of objects, there is a (not uncommon) usecase for supporting `Blob` objects: `ngMock`'s `$httpBackend` will return a copy of the response data (so that changes in one test won't affect others). Since returning `Blob` objects in response to HTTP requests is a valid usecase and since `ngMocks`'s `$httpBackend` will use `copy()` to create a copy of that data, it is reasonable to support `Blob` objects. (I didn't run any benchmarks, but the additional check for the type of the copied element should have negligible impact, compared to the other stuff that `copy()` is doing.) Fixes angular#9669
| expect(dst).not.toBe(src); | ||
| expect(dst.size).toBe(3); | ||
| expect(dst.type).toBe('bar'); | ||
| expect(isBlob(dst)).toBe(true); |
There was a problem hiding this comment.
Have you checked if this test perhaps passes and gives a false positive without the applied copyType() patch?
I seem to recall, when running into this issue first, that the cloned object was getting reported as a Blob even though it was not actually one, and I could only detect the difference by attempting to make a call to one of its specific functions like slice() or by attempting to read the blob's content by using the FileReader interface (either of which then failed in some internal implementation, in different ways depending on the browser used).
Update: I just checked it at http://plnkr.co/edit/xenwdKodooTrYO7VG2P2 and the test is good while my memory was mistaken. 👍 😄 The test will fail on 3 of those expect() calls - size access, type access & the isBlob() check. The thing I recalled incorrectly was that the typeof operator was returning Blob for the cloned object. Good work!
There was a problem hiding this comment.
BTW, I don't think typeof returns Blob for blobs. In latest Chrome and Firefox at least, it return object.
(Maybe what you had in mind is that expect(typeof dst).toBe(typeof src) would indeed pass even without the fix (because both would return object).)
There was a problem hiding this comment.
I'm certain I got Blob for natively created Blob objects with PhantomJS. Don't recall what actual user browsers gave me there. But it does not matter in the end. Should work like a charm now. 😺
|
Approved. Tested it, and it works as advertised for me. 👍 Not sure how you report review approvals here. 😄 Any chance this can be back-ported to the 1.4.x branch? |
|
LGTM
|
| @@ -1011,6 +1011,26 @@ describe('ngMock', function() { | |||
| }); | |||
|
|
|||
|
|
|||
| it('should be able to handle Blobs as mock data', function() { | |||
| if (typeof Blob !== 'udefined') { | |||
Although `copy()` does not need to (and never will) support all kinds of objects, there is a (not uncommon) usecase for supporting `Blob` objects: `ngMock`'s `$httpBackend` will return a copy of the response data (so that changes in one test won't affect others). Since returning `Blob` objects in response to HTTP requests is a valid usecase and since `ngMocks`'s `$httpBackend` will use `copy()` to create a copy of that data, it is reasonable to support `Blob` objects. (I didn't run any benchmarks, but the additional check for the type of the copied element should have negligible impact, compared to the other stuff that `copy()` is doing.) Fixes #9669 Closes #14064
Although `copy()` does not need to (and never will) support all kinds of objects, there is a (not uncommon) usecase for supporting `Blob` objects: `ngMock`'s `$httpBackend` will return a copy of the response data (so that changes in one test won't affect others). Since returning `Blob` objects in response to HTTP requests is a valid usecase and since `ngMocks`'s `$httpBackend` will use `copy()` to create a copy of that data, it is reasonable to support `Blob` objects. (I didn't run any benchmarks, but the additional check for the type of the copied element should have negligible impact, compared to the other stuff that `copy()` is doing.) Fixes #9669 Closes #14064
Although
copy()does not need to (and never will) support all kinds of objects, there is a (not uncommon) usecase for supportingBlobobjects:ngMock's$httpBackendwill return a copy of the response data (so that changes in one test won't affect others). Since returningBlobobjects in response to HTTP requests is a valid usecase and sincengMocks's$httpBackendwill usecopy()to create a copy of that data, it is reasonable to supportBlobobjects.(I didn't run any benchmarks, but the additional check for the type of the copied element should have negligible impact, compared to the other stuff that
copy()is doing.)Fixes #9669